Use enormous union type to make message types stricter #1075
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
EzraBrooks
force-pushed
the
use-enormous-union-type-for-message-types
branch
from
df49a5c to
0183a0d
Compare
EzraBrooks
force-pushed
the
use-enormous-union-type-for-message-types
branch
from
0183a0d to
fb62e70
Compare
|
of course right after I tell @douglascayers it's not necessary to expose all these types I go and make the types more exhaustive and it turns out I was wrong 😆 |
|
This basically accomplishes a bunch of what I was doing in my other PR so it's fine by me :) Couple things for consideration maybe?
This also doesn't provide typing for service or action events emitted by the Ros client. |
|
Yeah, that would be great - I was just about to comment on your PR that I hope you don't take my jumping in and fixing these issues in a slightly different way as my not appreciating your contribution. There are definitely some other great fixes and doc improvements in your PR! |
|
Oh no offense taken at all! The renaming was mostly just because I already had the type file from a previous PR I didn't get to submit, and like you said -- it's tedious lol. |
src/core/Action.ts
Outdated
| setFailed(id: string) { | ||
| const call = { | ||
| op: "action_result", | ||
| op: "action_result" as const, |
There was a problem hiding this comment.
curious, why was as const necessary?
There was a problem hiding this comment.
oop. thanks for catching that. moving this inline to the invocation of callOnConnection like I did elsewhere fixes the problem in a more obvious way. using as const helped TypeScript understand which message type it was, but I should've either used a type annotation or passed it directly into the parameter
There was a problem hiding this comment.
moved it inline
Fixes #1069 and a couple other things we hadn't caught yet.
EzraBrooks
force-pushed
the
use-enormous-union-type-for-message-types
branch
from
fb62e70 to
8f94936
Compare
Fixes #1069 and a couple other things we hadn't caught yet. (also, validates my instinct that
= unknownfor type args is a lot less painful for static typing than= undefined, because I had to change some of them from the latter to the former for this to work)